Skip to content

Conversation

@mashraf-222
Copy link
Contributor

@mashraf-222 mashraf-222 commented Nov 26, 2025

PR Type

Bug fix, Enhancement


Description

  • Validate tests_root directory existence

  • Support dict and object config access

  • Improve error response for invalid path

  • Use config-relative path resolution


Diagram Walkthrough

flowchart LR
  CFG["Config (dict or object)"]
  GET["get_config_value helper"]
  FILE["config file path (optional)"]
  BASE["Resolve base dir (cfg file or CWD)"]
  TR["tests_root path"]
  VAL["Validate directory exists"]
  ERR["Error response with field_errors"]
  SET["Build VsCodeSetupInfo"]

  CFG -- "access keys via" --> GET
  GET -- "tests_root" --> TR
  FILE -- "determine base dir" --> BASE
  BASE -- "join + resolve" --> TR
  TR -- "exists & is_dir?" --> VAL
  VAL -- "no" --> ERR
  VAL -- "yes" --> SET
Loading

File Walkthrough

Relevant files
Enhancement
beta.py
Add tests_root validation and config accessor                       

codeflash/lsp/beta.py

  • Add helper to read config as dict/object.
  • Validate provided tests_root directory exists.
  • Resolve tests_root relative to config file or CWD.
  • Use unified accessor when building VsCodeSetupInfo.
+28/-4   

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot changed the title lsb tests validation lsb tests validation Nov 26, 2025
@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Cross‑platform Path Formatting

The error messages embed the resolved path directly; on Windows this may produce backslashes and drive letters. Ensure downstream consumers (e.g., VS Code UI) handle these formats consistently or consider normalizing to string via as_posix() for predictable messaging.

return {
    "status": "error",
    "message": f"Invalid 'tests_root': directory does not exist at {tests_root_path}",
    "field_errors": {
        "tests_root": f"Directory does not exist at {tests_root_path}",
    },
}
Silent Defaulting

get_config_value defaults to empty strings for keys like module_root and tests_root; this may hide misconfigurations. Consider validating empty values or distinguishing “missing” vs “empty” to provide clearer feedback.

def get_config_value(key: str, default: str = "") -> str:
    if isinstance(cfg, dict):
        return cfg.get(key, default)
    return getattr(cfg, key, default)

tests_root = get_config_value("tests_root", "")
# Validate tests_root directory exists if provided
if tests_root:
    # Resolve path relative to config file directory or current working directory
    if cfg_file:
        base_dir = cfg_file.parent
    else:
        base_dir = Path.cwd()
    tests_root_path = (base_dir / tests_root).resolve()
    if not tests_root_path.exists() or not tests_root_path.is_dir():
        return {
            "status": "error",
            "message": f"Invalid 'tests_root': directory does not exist at {tests_root_path}",
            "field_errors": {
                "tests_root": f"Directory does not exist at {tests_root_path}",
            },
        }

setup_info = VsCodeSetupInfo(
    module_root=get_config_value("module_root", ""),
    tests_root=tests_root,
    test_framework=get_config_value("test_framework", "pytest"),
    formatter=get_formatter_cmds(get_config_value("formatter_cmds", "disabled")),
)

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle absolute tests_root paths

If tests_root is an absolute path, prepending base_dir yields an incorrect path.
Normalize by using Path(tests_root) directly and only join when it is relative. This
avoids false negatives when users provide absolute directories.

codeflash/lsp/beta.py [193-209]

 tests_root = get_config_value("tests_root", "")
 # Validate tests_root directory exists if provided
 if tests_root:
-    # Resolve path relative to config file directory or current working directory
-    if cfg_file:
-        base_dir = cfg_file.parent
-    else:
-        base_dir = Path.cwd()
-    tests_root_path = (base_dir / tests_root).resolve()
+    tr_path = Path(tests_root)
+    if not tr_path.is_absolute():
+        base_dir = cfg_file.parent if cfg_file else Path.cwd()
+        tr_path = (base_dir / tr_path)
+    tests_root_path = tr_path.resolve()
     if not tests_root_path.exists() or not tests_root_path.is_dir():
         return {
             "status": "error",
             "message": f"Invalid 'tests_root': directory does not exist at {tests_root_path}",
             "field_errors": {
                 "tests_root": f"Directory does not exist at {tests_root_path}",
             },
         }
Suggestion importance[1-10]: 7

__

Why: Good catch: current join with base_dir breaks absolute paths; the fix is precise and prevents false errors while preserving relative resolution behavior.

Medium
Preserve original config types

The helper returns values typed as string, but config fields can be non-strings
(e.g., dict for formatter or None). This can cause unintended string defaults or
attribute fallback behavior. Remove the string type hints and defaults to preserve
original types and avoid silent coercion.

codeflash/lsp/beta.py [188-192]

-def get_config_value(key: str, default: str = "") -> str:
+def get_config_value(key: str, default=None):
     if isinstance(cfg, dict):
         return cfg.get(key, default)
     return getattr(cfg, key, default)
Suggestion importance[1-10]: 6

__

Why: Correctly identifies that the helper enforces string defaults/types which may be wrong for non-string config fields; proposed change is minimal and accurate, improving type fidelity without altering logic.

Low
General
Normalize formatter disabled value

Passing string "disabled" through get_formatter_cmds may clash with expected types;
ensure disabled state is represented consistently (e.g., None) before
transformation. Map falsy/disabled values to a clear sentinel to prevent
misconfiguration.

codeflash/lsp/beta.py [211-216]

+formatter_cfg = get_config_value("formatter_cmds", None)
+if formatter_cfg in ("disabled", "", False):
+    formatter_cfg = None
 setup_info = VsCodeSetupInfo(
     module_root=get_config_value("module_root", ""),
     tests_root=tests_root,
     test_framework=get_config_value("test_framework", "pytest"),
-    formatter=get_formatter_cmds(get_config_value("formatter_cmds", "disabled")),
+    formatter=get_formatter_cmds(formatter_cfg),
 )
Suggestion importance[1-10]: 5

__

Why: Reasonable improvement to normalize "disabled" before passing to get_formatter_cmds, but impact depends on how get_formatter_cmds already handles such values; beneficial yet moderately important.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants